Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Keep a record of stale owners to help better debug poor test isolation #20513

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Jul 29, 2023

This code comes from some internal code that is meant to help track down leaky tests and identify which tests are leaking.

This is useful code, but the patch is complicated enough where upgrading is a bit of a pain, so I'm putting this PR up in collaboration with @runspired and @wagenet to help upstream this ability, and help make it more robust for broader adoption.

Notes:

  • I'm not convinced we need this code outside of tests, so while non-functional in this first draft, I've wrapped the changes in macros (tho, it maayyybe? makes sense to not have these changes wrapped in macros if we want to better support the use case of multiple ephemeral ember apps on a single page outside of tests)
  • "somehow", we need to set up the Map of owners => test infos.
    in the internal code, there is:
    // tests/helpers/some-file.js
    const OWNER_MAP = new Map();
    // we expose this for custom assert handling
    window.OWNER_MAP = OWNER_MAP;
    I'm not convinced this needs to be in a tests file, nor pushed in to the app, but potentially could be initialized by ember-source, and we change the Map to a WeakMap, so that memory is automatically freed when owner references has no more... references.

Everything is open to discussion of course. I don't think this needs an RFC, because it could be considered a bugfix of how we handle test isolation (and booting / destroying multiple applications), but it is still worth discussion design of this behavior (I also considered most things that are DX enhancements to rough edges of a library/framework to be bugfixes in general -- the bug being fixed is suboptimal error comprehension).

The patch I'm trying to get rid of
diff --git a/CHANGELOG.md b/CHANGELOG.md
deleted file mode 100644
index 509ef7759ab62a660b420c41caab442c9f4b5a2d..0000000000000000000000000000000000000000
diff --git a/dist/ember.debug.js b/dist/ember.debug.js
index c362eb82209d5eeac5e741740e80d9553e373279..bb2b4473cbaa1f6f3300d0fa533ca8bf219c02e7 100644
--- a/dist/ember.debug.js
+++ b/dist/ember.debug.js
@@ -232,7 +232,10 @@ define("@ember/-internals/container/index", ["exports", "@ember/-internals/owner
      */
     lookup(fullName, options) {
       if (this.isDestroyed) {
-        throw new Error(`Cannot call \`.lookup('${fullName}')\` after the owner has been destroyed`);
+        var _a = window.OWNER_MAP;
+        var guid = (0, _utils.guidFor)(this.owner);
+        var testInfo = _a === null || _a === void 0 ? void 0 : _a.get(guid);
+        throw new Error(`Cannot call \`.lookup('${fullName}')\` after the owner has been destroyed. owner is ${guid} (test ${testInfo === null || testInfo === void 0 ? void 0 : testInfo.testId} ${testInfo === null || testInfo === void 0 ? void 0 : testInfo.name})`);      
       }
       (true && !(this.registry.isValidFullName(fullName)) && (0, _debug.assert)('fullName must be a proper full name', this.registry.isValidFullName(fullName)));
       return lookup(this, this.registry.normalize(fullName), options);
@@ -8738,7 +8741,16 @@ define("@ember/-internals/metal/index", ["exports", "@ember/-internals/meta", "@
     (true && !(typeof keyName === 'string' || typeof keyName === 'number' && !isNaN(keyName)) && (0, _debug.assert)(`The key provided to set must be a string or number, you passed ${keyName}`, typeof keyName === 'string' || typeof keyName === 'number' && !isNaN(keyName)));
     (true && !(typeof keyName !== 'string' || keyName.lastIndexOf('this.', 0) !== 0) && (0, _debug.assert)(`'this' in paths is not supported`, typeof keyName !== 'string' || keyName.lastIndexOf('this.', 0) !== 0));
     if (obj.isDestroyed) {
-      (true && !(tolerant) && (0, _debug.assert)(`calling set on destroyed object: ${(0, _utils.toString)(obj)}.${keyName} = ${(0, _utils.toString)(value)}`, tolerant));
+      if (!tolerant) {
+        var owner = (0, _owner.getOwner)(obj);
+        if (owner) {
+          var guid = (0, _utils.guidFor)(owner);
+          var testInfo = (_a = window.OWNER_MAP) === null || _a === void 0 ? void 0 : _a.get(guid);
+          (true && !(tolerant) && (0, _debug.assert)(`calling set on destroyed object: ${(0, _utils.toString)(obj)}.${keyName} = ${(0, _utils.toString)(value)} owned by ${guid} (test ${testInfo === null || testInfo === void 0 ? void 0 : testInfo.testId} ${testInfo === null || testInfo === void 0 ? void 0 : testInfo.name})`, tolerant));
+        } else {
+          (true && !(tolerant) && (0, _debug.assert)(`calling set on destroyed object: ${(0, _utils.toString)(obj)}.${keyName} = ${(0, _utils.toString)(value)}`, tolerant));
+        }
+      }
       return value;
     }
     return isPath(keyName) ? _setPath(obj, keyName, value, tolerant) : _setProp(obj, keyName, value);
diff --git a/dist/packages/@ember/-internals/container/index.js b/dist/packages/@ember/-internals/container/index.js
index b52c87df90cd144bd0b165e3cd467dd9a6e0d01a..1ce42d07cd3c4787ffa3569e9fa68f1adfdcb9ed 100644
--- a/dist/packages/@ember/-internals/container/index.js
+++ b/dist/packages/@ember/-internals/container/index.js
@@ -1,5 +1,5 @@
 import { setOwner } from '@ember/-internals/owner';
-import { dictionary, intern } from '@ember/-internals/utils';
+import { dictionary, guidFor, intern } from '@ember/-internals/utils';
 import { assert, deprecate } from '@ember/debug';
 import { DEBUG } from '@glimmer/env';
 
@@ -111,7 +111,11 @@ class Container {
    */
   lookup(fullName, options) {
     if (this.isDestroyed) {
-      throw new Error(`Cannot call \`.lookup('${fullName}')\` after the owner has been destroyed`);
+      const _a = window.OWNER_MAP;
+      const guid = guidFor(this.owner);
+      const testInfo = _a === null || _a === void 0 ? void 0 : _a.get(guid);
+      throw new Error(`Cannot call \`.lookup('${fullName}')\` after the owner has been destroyed. owner is ${guid} (test ${testInfo === null || testInfo === void 0 ? void 0 : testInfo.testId} ${testInfo === null || testInfo === void 0 ? void 0 : testInfo.name})`);
+     
     }
     assert('fullName must be a proper full name', this.registry.isValidFullName(fullName));
     return lookup(this, this.registry.normalize(fullName), options);
diff --git a/dist/packages/@ember/-internals/metal/index.js b/dist/packages/@ember/-internals/metal/index.js
index f63d59a38f6f1707ff03a9fe3d61a30a79763eb3..43ab04017a8befa8e51dc8ca9000780feecf4541 100644
--- a/dist/packages/@ember/-internals/metal/index.js
+++ b/dist/packages/@ember/-internals/metal/index.js
@@ -1,5 +1,5 @@
 import { meta, peekMeta } from '@ember/-internals/meta';
-import { setListeners, isObject, setupMandatorySetter, symbol, toString, setWithMandatorySetter, Cache, setProxy, lookupDescriptor, getName, setName } from '@ember/-internals/utils';
+import { setListeners, isObject, setupMandatorySetter, symbol, toString, setWithMandatorySetter, Cache, setProxy, lookupDescriptor, guidFor, getName, setName } from '@ember/-internals/utils';
 import { assert, inspect, deprecate, debug, warn } from '@ember/debug';
 import { ENV, context } from '@ember/-internals/environment';
 import { schedule } from '@ember/runloop';
@@ -1695,6 +1695,17 @@ function set(obj, keyName, value, tolerant) {
   assert(`'this' in paths is not supported`, typeof keyName !== 'string' || keyName.lastIndexOf('this.', 0) !== 0);
   if (obj.isDestroyed) {
     assert(`calling set on destroyed object: ${toString(obj)}.${keyName} = ${toString(value)}`, tolerant);
+    if (!tolerant) {
+      const owner = getOwner(obj);
+      if (owner) {
+        const _a = window.OWNER_MAP;
+        const guid = guidFor(owner);
+        const testInfo = _a === null || _a === void 0 ? void 0 : _a.get(guid);
+        assert(`calling set on destroyed object: ${toString(obj)}.${keyName} = ${toString(value)} owned by ${guid} (test ${testInfo === null || testInfo === void 0 ? void 0 : testInfo.testId} ${testInfo === null || testInfo === void 0 ? void 0 : testInfo.name})`, tolerant);
+      } else {
+        assert(`calling set on destroyed object: ${toString(obj)}.${keyName} = ${toString(value)}`, tolerant);
+      }
+    }
     return value;
   }
   return isPath(keyName) ? _setPath(obj, keyName, value, tolerant) : _setProp(obj, keyName, value);
diff --git a/dist/packages/@ember/-internals/runtime/.gitignore b/dist/packages/@ember/-internals/runtime/.gitignore
deleted file mode 100644
index a1136368651e6eb6d0d93a09c478f4978f4196fa..0000000000000000000000000000000000000000

…s at AB so we can move off the patch ember-source, and that upgrades become easier
@runspired
Copy link
Contributor

Additional context, this is what we do with that map

	hooks.beforeEach(function (assert) {
		const owner = this.owner;
		OWNER_MAP.set(guidFor(owner), {
			testId: assert.test.testId,
			name: assert.test.module.name + ' | ' + assert.test.testName,
		});
	});

This makes it much easier to trace down what test kicked off some async that leaked into other tests and caused a failure later.

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Jul 29, 2023

in ember-qunit, we can pre-wire that in a QUnit.startStart hook.

The code in this PR will need to be resilient to a test-adapter forgetting / opting out of "container test-reporting" (what I'm calling this feature)

To do that though, we need to get the ember-qunit@v8 release out -- blocked on this (should be unblocked next ember-cli release cycle).

@chriskrycho
Copy link
Contributor

Potentially useful reference code along the same lines, pulled straight out of the big app at LinkedIn (which @rwjblue and @nlfurniss and I built earlier this year)—note that it's super hacky in a couple ways, particularly around the way it does shenanigans to get an actual class name into place for the ApplicationInstance subclass, which shouldn't be necessary on current versions of Ember (it's a pre-4.x limitation); and around its monkey-patching-in owner tracking. However, it does do the job effectively, and we used it to track down quite a few leaks in tests (which were also affecting the app in FastBoot).

owner-leak-detector.js
/* eslint-disable no-restricted-syntax */
/* global WeakRef, gc */

import QUnit from 'qunit';
import Application from '@ember/application';
import Engine from '@ember/engine';
import * as ApplicationInstanceModule from '@ember/application/instance';

const HAS_GC = typeof gc === 'function';

let OWNER_REFS = [];

async function fullyGC() {
  await gc({ type: 'major', execution: 'async' });
  await gc({ type: 'major', execution: 'async' });
  await gc({ type: 'major', execution: 'async' });
}

async function checkForRetainedOwners() {
  if (!HAS_GC) {
    throw new Error(
      "You're running Owner leak detection but GC is not enabled!"
    );
  }

  await fullyGC();

  const testNameToRetainedOwner = new Map();

  for (let i = 0; i < OWNER_REFS.length; i++) {
    const [ownerRef, testName] = OWNER_REFS[i];
    const owner = ownerRef.deref();
    if (owner !== undefined) {
      let owners = testNameToRetainedOwner.get(testName);
      if (owners === undefined) {
        owners = [];
        testNameToRetainedOwner.set(testName, owners);
      }
      owners.push(owner);
    }
  }

  OWNER_REFS = [];

  return testNameToRetainedOwner;
}

function getOwnerMetadata(owner) {
  if (owner.mountPoint) {
    return `Engine [mounted at \`/${owner.mountPoint}\`]`;
  }
  return `Application`;
}

function getTestName() {
  const currentTest = QUnit.config.current;
  return `${currentTest.module.name}: ${currentTest.testName}`;
}

function trackOwner(owner) {
  OWNER_REFS.push([new WeakRef(owner), getTestName()]);
}

function applyMonkeyPatchesToCaptureOwners() {
  // eslint-disable-next-line global-require, no-undef
  require('voyager-web/instance-initializers/auto-engine-dependencies');

  // The block below is done to add a class name to the ApplicationInstanceInstance that isn't present in Ember versions less than 4.x.
  // This class name makes it easier to find the owner (needle) in the haystack.
  // This can be removed once the Ember 4.x upgrade is done
  const ApplicationInstance = ApplicationInstanceModule.default;
  ApplicationInstance.proto();
  class VWebApplicationInstance extends ApplicationInstanceModule.default {}
  // eslint-disable-next-line no-import-assign
  ApplicationInstanceModule.default = VWebApplicationInstance;

  Application.proto(); // This is needed to setup the prototype in Ember 3.28
  const originalBuildApplicationInstance = Application.prototype.buildInstance;
  function ApplicationBuildInstanceOverride(options) {
    const owner = originalBuildApplicationInstance.call(this, options);
    trackOwner(owner);
    return owner;
  }

  ApplicationBuildInstanceOverride.isOwnerCaptureMonkeyPatch = true;

  Engine.proto(); // This is needed to setup the prototype in Ember 3.28
  const originalBuildEngineInstance = Engine.prototype.buildInstance;
  function EngineBuildInstanceOverride(options) {
    const owner = originalBuildEngineInstance.call(this, options);
    trackOwner(owner);
    return owner;
  }
  EngineBuildInstanceOverride.isOwnerCaptureMonkeyPatch = true;

  if (!originalBuildApplicationInstance.isOwnerCaptureMonkeyPatch) {
    // This is needed only as long as we have this in Voyager; when it is
    // upstreamed into open source libraries we will not need the monkey-patch.
    // eslint-disable-next-line @linkedin/pemberly/require-read-only-prototypes
    Application.prototype.buildInstance = ApplicationBuildInstanceOverride;
  }

  if (!originalBuildEngineInstance.isOwnerCaptureMonkeyPatch) {
    // This is needed only as long as we have this in Voyager; when it is
    // upstreamed into open source libraries we will not need the monkey-patch.
    // eslint-disable-next-line @linkedin/pemberly/require-read-only-prototypes
    Engine.prototype.buildInstance = EngineBuildInstanceOverride;
  }
}

function ensureTestReleasesTestEnvironment(callback = () => {}) {
  QUnit.on('testStart', () => {
    const currentTest = QUnit.config.current;
    const { finish } = currentTest;

    currentTest.finish = async function () {
      await callback();

      return finish.apply(this, arguments);
    };
  });
}

/**
  After each test is completed, check for owner leaks. This will be the best
  ergonomics (the actual test that leaks will fail), but due to running GC many
  times per test, will be the slowest mechanism.
*/
export function setupPerTestLeakDetection() {
  applyMonkeyPatchesToCaptureOwners();
  ensureTestReleasesTestEnvironment(async function () {
    const currentTest = QUnit.config.current;
    for (const [, owners] of await checkForRetainedOwners()) {
      for (const owner of owners) {
        currentTest.expected++;
        currentTest.assert.pushResult({
          result: false,
          message: `Leaked ${getOwnerMetadata(owner)}`,
        });
      }
    }
  });
}

let hasDonePerModuleLeakDetection = false;

/**
  After each module is completed, check for owner leaks. This will be faster than
  checking for each test, but slower than checking once at the end of all tests.
*/
export function setupPerModuleLeakDetection() {
  applyMonkeyPatchesToCaptureOwners();
  ensureTestReleasesTestEnvironment();

  QUnit.on('suiteEnd', () => {
    if (hasDonePerModuleLeakDetection) return;
    hasDonePerModuleLeakDetection = true;

    QUnit.module('[OWNER LEAK DETECTION]', function () {
      const testBody = async function (assert) {
        assert.expect(0);
        for (const [testName, owner] of await checkForRetainedOwners()) {
          assert.pushResult({
            result: false,
            message: `Leaked ${getOwnerMetadata(owner)} from ${testName}`,
          });
        }
      };
      // Opts the user into the `OWNER LEAK DETECTION` module regardless of filter or module selected.
      testBody.validTest = true;

      QUnit.test('There should be zero leaked Owners', testBody);
    });
  });
}

let hasCheckedOwnerLeaks = false;

/**
  After all tests have been ran, check for any owner leaks. This will be the fastest
  mechanism, but also will be ran the least often (you won't get feedback until all
  tests have completed).
*/
export function setupAfterAllTestsOwnerLeakDetection() {
  applyMonkeyPatchesToCaptureOwners();
  ensureTestReleasesTestEnvironment();

  // Due to details of how QUnit functions (see https://github.com/qunitjs/qunit/pull/1629)
  // we cannot enqueue new tests if we use `runEnd` which is basically what we want (i.e. "when all tests are done run this callback")
  //
  // Instead we use `suiteEnd` and check `QUnit.config.queue` which indicates the number of tests remaining to be ran
  // when `QUnit.config.queue` gets to `0` and `suiteEnd` is running we are finished with all tests **but** `runEnd` / `QUnit.done()` hasn't ran yet so we can still emit new tests
  QUnit.on('suiteEnd', () => {
    if (QUnit.config.queue.length !== 0) {
      return;
    }

    if (hasCheckedOwnerLeaks) return;
    hasCheckedOwnerLeaks = true;

    QUnit.module(`[OWNER LEAK DETECTION]`, function () {
      const testBody = async function (assert) {
        assert.expect(0);

        const leakedOwners = await checkForRetainedOwners();
        for (const [testName, owners] of leakedOwners) {
          for (const owner of owners) {
            assert.pushResult({
              result: false,
              message: `${testName}: Leaked ${getOwnerMetadata(owner)}`,
            });
          }
        }
      };
      // Opts the user into the `OWNER LEAK DETECTION` module regardless of filter or module selected.
      testBody.validTest = true;

      QUnit.test('There should be zero leaked Owners', testBody);
    });
  });
}

@runspired
Copy link
Contributor

@chriskrycho out of curiosity, how have you enabled gc? All of the v8 flags that are supposedly required have not worked for us to activate the feature any more. We're starting chrome via testem with this flag currently --js-flags="--allow-natives-syntax --expose-gc"

Using a WeakRef for this is clever, though I will say the approach I've taken for this at AB that Preston is looking to get supported here is significantly cheaper and works for leaks that don't retain owner as well.

@kategengler
Copy link
Member

Is this still relevant? It is a draft PR

@NullVoxPopuli
Copy link
Contributor Author

yeah, it's a problematic test debugging improvement, but needs more thought as to implementation rollout -- it's part of a goal I have to remove internal patches :_|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants